Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create new Federation and the complete process until it is confirmed and activated #2925

Merged
merged 26 commits into from
Feb 4, 2025
Merged

Create new Federation and the complete process until it is confirmed and activated #2925

merged 26 commits into from
Feb 4, 2025
+600 −0

Conversation

apancorb
Copy link
Contributor

@apancorb apancorb commented Jan 10, 2025 •
edited
Loading

Description

The idea is to provide building blocks that can be reused while creating new tests. This focuses on creating a new active and retiring federation.

@apancorb apancorb self-assigned this Jan 10, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 10, 2025 •
edited
Loading

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

@apancorb apancorb changed the title Fix/powpeg migration tests common path Create new Federation and the complete process until it is confirmed and activated Jan 10, 2025
@apancorb apancorb marked this pull request as ready for review January 10, 2025 15:33
@apancorb apancorb requested a review from a team as a code owner January 10, 2025 15:33
jeremy-then
jeremy-then reviewed Jan 10, 2025
View reviewed changes
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Outdated
case P2SH_ERP -> {
originalFederation = FederationFactory.buildP2shErpFederation(
originalFederationArgs, erpPubKeys, activationDelay);
// TODO: CHECK REDEEMSCRIPT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this TODO be here still?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, thanks

jeremy-then
jeremy-then approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@jeremy-then jeremy-then left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch from 1ba5cc7 to 1191212 Compare January 14, 2025 10:48
julia-zack
julia-zack approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@julia-zack julia-zack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work

@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch 3 times, most recently from baf74f1 to 89d811f Compare January 30, 2025 10:50
@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch from 89d811f to dcadcb4 Compare January 30, 2025 12:18
@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch from 22efa4a to dc6fb3f Compare January 31, 2025 09:16
marcos-iov
marcos-iov reviewed Jan 31, 2025
View reviewed changes
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Show resolved Hide resolved
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Outdated Show resolved Hide resolved
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Outdated Show resolved Hide resolved
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Outdated Show resolved Hide resolved
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Outdated Show resolved Hide resolved
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Outdated Show resolved Hide resolved
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java
newFederation.getAddress(), originalFederation.getAddress());
assertMigrationHasStarted();
verifySigHashes();
verifyPegoutTransactionCreatedEventWasEmitted();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other events that should be emitted as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the verification for verifyReleaseBtcRequestedEventEventWasEmitted. Let me know if that is good. Or you were thinking on something else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

release_requested, pegout_confirmed, pegout_transaction_created , addSignature (per signature of the migration transaction), release_btc

I think those are the events emitted for a migration transaction. Check with @jeremy-then if there is any docs that describe the complete process. I might be missing some

Copy link
Contributor Author

@apancorb apancorb Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to include pegout_confirmed event as well. I checked that the addSignature and release_btc events are not being emmitted because the ending migration transaction is never being signed. I also checked on how it was being done here and it is also not adding signatures to the ending migration tx. Should we include this logic here as well?

Let me check with @jeremy-then for any missing events.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good. We can look into adding more validations in later stages. Thanks

@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch from 24155fa to f0224a8 Compare February 4, 2025 10:17
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Feb 4, 2025

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@marcos-iov marcos-iov merged commit e17fddb into federation_change_federeration_integration Feb 4, 2025
9 checks passed
@marcos-iov marcos-iov deleted the fix/powpeg-migration-tests-common-path branch February 4, 2025 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremy-then jeremy-then jeremy-then approved these changes

@julia-zack julia-zack julia-zack approved these changes

@marcos-iov marcos-iov marcos-iov approved these changes

Assignees

@apancorb apancorb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apancorb @marcos-iov @julia-zack @jeremy-then